Skip to content

fix(runtime): rewrite nested identity when copying model call attempts - #3776

Merged
Astro-Han merged 2 commits into
apache:mainfrom
liuxiaocs7:fix/conversation-copy-model-call-attempt-identity
Aug 25, 2026
Merged

fix(runtime): rewrite nested identity when copying model call attempts#3776
Astro-Han merged 2 commits into
apache:mainfrom
liuxiaocs7:fix/conversation-copy-model-call-attempt-identity

Conversation

@liuxiaocs7

Copy link
Copy Markdown
Member

Summary

cloneAgentRunEvent in packages/runtime/src/conversation-copy.ts rewrote the envelope identity of a copied model_call_attempt_recorded event but left the nested ModelCallAttempt payload at the source sessionId/runId/attemptId. The model-call projection guard (packages/storage/src/model-call-ledger.ts:293) then rejected the copied attempt as unreadable because its payload disagreed with the rewritten envelope, so a branched / side / revised session's spend silently dropped and Daily Review reported the canonical usage projection as incomplete ("Daily Review is waiting for canonical Usage repair").

Rewriting only sessionId/runId would make that guard pass but expose a second hazard: attemptId is the ledger's global primary key, so reusing the source value would let the copy overwrite the source session's own accounting row.

This adds a model_call_attempt_recorded branch that rewrites the owned payload identity the same way the sibling provider-request rewriters do:

  • sessionId / runId → target,
  • attemptId → the fresh envelope id (preserving the event.id === attemptId writer contract),
  • traceId → the copy's provider-trace map (extended to also collect model-call traces),
  • logicalCallId → a new per-copy logical-call map (keeps retries of one call grouped in the target),
  • captureArtifactIdrewriteOwnedArtifactId,
  • turnId is unchanged (the envelope turnId is not rewritten, so they still agree).

Invalid payloads are rejected via decodeModelCallAttempt, matching the existing provider-request rewriters.

Fixes #3775

Verification

  • npm --workspace @maka/runtime run typecheck — clean
  • npx biome check on the two changed files — clean
  • packages/runtime/src/__tests__/conversation-copy.test.js — 18/18 pass, including the new regression test conversation copy rewrites the nested identity of a model call attempt, which asserts the copied event's envelope and nested payload identity both move to the target and that the record still decodes. It fails on main (the payload keeps the source identity).

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Claude Code (Claude Opus 4.8) — diagnosed the root cause, authored the rewriteModelCallAttempt rewriter and the identity-map plumbing, and wrote the regression test. Human-reviewed. The commit carries a Generated-by: Claude Code trailer.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

cloneAgentRunEvent rewrote the envelope identity of a copied
model_call_attempt_recorded event but left the nested ModelCallAttempt
payload at the source sessionId/runId/attemptId. The model-call
projection guard then rejected the attempt as unreadable — its payload
disagreed with the rewritten envelope — so a branched/side/revised
session's spend silently dropped and daily review reported the canonical
usage projection as incomplete.

Rewriting only sessionId/runId would make that guard pass but expose a
second hazard: attemptId is the ledger's global primary key, so reusing
the source value would let the copy overwrite the source session's own
accounting row. So map the payload sessionId/runId to the target, the
attemptId to the fresh envelope id, and the owned traceId/logicalCallId/
captureArtifactId through the copy's identity maps, mirroring the sibling
provider-request rewriters. Add a regression test covering the rewrite.

Generated-by: Claude Code

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this head and found no blocking issues.

The copy now correctly rewrites nested session/run/attempt identities to the target, using fresh IDs for attempt accounting and preserving logical-call grouping. No review comments were pre-read; schema-invalid and legacy broken copies are rejected.

Checks on a0952bfab4 are test: success.

简体中文该头未发现阻断。

Astro-Han
Astro-Han previously approved these changes Aug 25, 2026

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVE — a0952bf

@Astro-Han
Astro-Han dismissed their stale review August 25, 2026 09:25

Superseded by new P2 — stranded legacy copy

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update on a0952bfab4: blocking issue found that strands previously copied Sessions.

[P2] Stranded legacy copy after upgrade

Parent cloneAgentRunEvent created a new envelope but left data.attemptId/sessionId/runId pointing at source. Current rewriteModelCallAttempt:797 throws Cannot copy invalid model call attempt before rewriting, so Sessions copied before the fix can never be copied again. Treat stale nested identity as repairable legacy state or provide migration.

Checks on a0952bfab4 remain test: success but this compatibility gap is independent of CI.

简体中文存在旧副本升级后无法再次拷贝的阻断。

The model-call-attempt rewriter asserted the writer contract
`attempt.attemptId === event.id` on the *source* event before rewriting.
That contract does not hold for sessions copied before this rewriter
existed: the old copy path had no branch for model_call_attempt_recorded,
so it rewrote the envelope id but left the nested payload — including
attemptId — at the source identity. Copying such a session again threw
`Cannot copy invalid model call attempt`, stranding it: it could never be
copied again.

Drop the assertion. The rewrite already reassigns sessionId/runId to the
target and attemptId to the fresh envelope id, and the trace/logical-call
maps are built from the same payload fields, so a stale nested identity is
repaired regardless of the source's state and the output still satisfies
`event.id === attemptId`. decodeModelCallAttempt still rejects a
schema-invalid payload, matching the sibling rewriters. Add a regression
test that copies an attempt whose nested identity was stranded by a pre-fix
copy and asserts the copy succeeds with a repaired, self-consistent identity.

Generated-by: Claude Code
@liuxiaocs7

Copy link
Copy Markdown
Member Author

Thanks for catching the [P2] — fixed in e2b5d8eb0.

Root cause of the compatibility gap: the rewriter asserted the writer contract attempt.attemptId === event.id on the source event. That contract holds for freshly-written attempts, but not for sessions copied before this rewriter existed: the old copy path had no model_call_attempt_recorded branch, so it rewrote the envelope id while leaving the nested payload (attemptId/sessionId/runId) at the source identity. Re-copying such a session threw Cannot copy invalid model call attempt and stranded it — exactly the gap you flagged.

Fix: drop the source-side assertion and treat the stale nested identity as repairable. The rewrite already reassigns sessionId/runId → target and attemptId → the fresh envelope id, and the trace / logical-call maps are built from the same payload fields, so a stale nested identity is repaired regardless of the source's state and the output still satisfies event.id === attemptId. decodeModelCallAttempt still rejects a schema-invalid payload, matching the sibling provider-request rewriters.

Test: added conversation copy repairs a model call attempt stranded by a pre-fix copy, which copies an attempt whose nested identity points at a foreign "grandparent" session and asserts the copy succeeds with a repaired, self-consistent identity. It fails on the previous revision (Cannot copy invalid model call attempt) and passes now.

Verification: @maka/runtime typecheck clean · biome check on both changed files clean · conversation-copy.test 19/19 pass.

简体中文

已在 e2b5d8eb0 修复该 [P2]。根因是 rewriter 对源事件断言了写入契约 attempt.attemptId === event.id,而修复前被复制过的会话并不满足该契约(旧复制路径只改了信封 id,嵌套载荷仍停留在源身份),导致再次复制时抛 Cannot copy invalid model call attempt 而搁浅。

修复:去掉源侧断言,把陈旧嵌套身份当作可修复状态处理 —— 改写本就会把 sessionId/runId 重写为目标、attemptId 重写为新信封 id,输出仍满足 event.id === attemptId;decodeModelCallAttempt 仍拒绝 schema 非法载荷。新增回归测试覆盖"被修复前复制搁浅的 attempt",在旧版本失败、现版本通过。typecheck / biome / 19 项测试均通过。

@liuxiaocs7
liuxiaocs7 requested a review from Astro-Han August 25, 2026 10:10

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update on e2b5d8eb07:

No blocking issues. The stranded-copy guard now correctly rewrites stale attemptId identities instead of throwing, allowing sessions copied before the fix to be recopied. test: queued — code GO.

简体中文该头为兼容性修复,无阻断。

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read the diff and checked the premises this rests on — all hold:

  • requiredMappedId can't miss. cloneAgentRunEvent runs inside plan.operationalEvents.flatMap() (:358), the same collection both providerTraceIdMap and the new logicalModelCallIdMap seed from.
  • traceId, logicalCallId and attemptId are all required string on ModelCallAttempt, so there's no optional branch to fall through.
  • The turnId reasoning is right: ids carries only sessionId/runId/eventId, and the guard at model-call-ledger.ts:293 only compares sessionId/runId anyway.
  • Sharing the trace map with provider_request_* is correct — same tracker instance id.

The second test earns its place. Not asserting attempt.attemptId === event.id on the source, so sessions copied before this fix can still be copied, is the kind of thing that's easy to miss and expensive to discover later.

One structural note, not for this PR: isCopiedAgentRunEvent says the rewriters "know which of this build's payloads carry source-owned references", but the whitelist only tests isEmittedAgentRunEventType — whether the build emits the type, not whether anyone wrote a rewriter for it. main has three rewriter branches against 60+ allowed types, and #3775 is what the gap looks like: the payload rode through with source identity until Daily Review reported a repair backlog. Fixing this one doesn't close it. I'll open an issue to sweep for a second instance.

Heads-up: this and #3651 both touch cloneAgentRunEvent, so whichever lands second will need a rebase.

简体中文

读了 diff,把这个修复依赖的前提都核了一遍,都成立:

  • requiredMappedId 不会缺失。cloneAgentRunEventplan.operationalEvents.flatMap():358)里执行,与 providerTraceIdMap 和新的 logicalModelCallIdMap seed 的是同一个集合。
  • traceIdlogicalCallIdattemptIdModelCallAttempt 上都是必填 string,没有可选分支可漏。
  • turnId 那段推理是对的:ids 只带 sessionId/runId/eventId,而 model-call-ledger.ts:293 的 guard 本来也只比较 sessionId/runId。
  • provider_request_* 共用 trace 映射是对的——同一个 tracker instance id。

第二个测试很有价值。不对断言 attempt.attemptId === event.id,让本次修复之前复制过的会话仍然能再复制——这种事很容易漏,而且漏了之后代价很大。

一条结构性的观察,不是这个 PR 的事:isCopiedAgentRunEvent 说 rewriter「知道这个 build 的哪些 payload 带源持有的引用」,但白名单只测 isEmittedAgentRunEventType——这个 build 是否发出该类型,而不是有没有人给它写了 rewriter。main 上只有三个 rewriter 分支,白名单放行 60+ 个类型,而 #3775 就是这个缺口的样子:payload 带着源 identity 一路进了目标,直到 Daily Review 报修复积压才暴露。修好这一个并不能关掉它。我会开个 issue 排查有没有第二例。

提醒一句:这个和 #3651 都改了 cloneAgentRunEvent,后合的那个需要 rebase。

@Astro-Han

Copy link
Copy Markdown
Contributor

Swept the event types as promised — no second case at the AgentRun payload level. Found one on the RuntimeEvent side of the same file though: refs.parentOperationId is never rewritten while refs.operationId is, so Code Mode nesting breaks after a copy. Filed as #3800.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVE — e2b5d8e, no P0-P3 per latest review.

@Astro-Han
Astro-Han merged commit 892d8b1 into apache:main Aug 25, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Conversation copy leaves model_call_attempt_recorded payload at the source identity, breaking Daily Review

2 participants